Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TreeView] Migrate to emotion #25673

Merged
merged 11 commits into from
Apr 10, 2021
Merged

Conversation

tomasznguyen
Copy link
Contributor

@tomasznguyen tomasznguyen commented Apr 8, 2021

Related to #24405

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 8, 2021

Details of bundle changes

@material-ui/lab: parsed: +0.11% , gzip: +0.07%

Generated by 🚫 dangerJS against 94d4a52

@oliviertassinari oliviertassinari added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Apr 8, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first quick review. In packages/material-ui-lab/src/TreeView/TreeView.d.ts, Theme needs to be imported from @material-ui/core/styles.

packages/material-ui-lab/src/TreeView/TreeView.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/TreeView/TreeView.test.js Outdated Show resolved Hide resolved
tomasznguyen and others added 3 commits April 8, 2021 23:05
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@tomasznguyen
Copy link
Contributor Author

@oliviertassinari Tnx for the quick review. I fixed the imports. I'll have a look at the other errors tomorrow.

@tomasznguyen
Copy link
Contributor Author

@oliviertassinari I got stuck with migrating TreeView. When running yarn proptypes, the properties className, onBlur, onFocus, and onKeyDown are removed from TreeView.propTypes.

When replacing (for testing):

const TreeView = React.forwardRef(function TreeView(inProps, ref) {
  const { isRtl, ...props } = useThemeProps({ props: inProps, name: 'MuiTreeView' });

by:

const TreeView = React.forwardRef(function TreeView(props, ref) {

yarn proptypes adds the 4 properties to TreeView.propTypes.

I try to understand why this is happening, but till now without success. Do you have any suggestion for me where to look at?

@mnajdova
Copy link
Member

mnajdova commented Apr 9, 2021

@oliviertassinari I got stuck with migrating TreeView. When running yarn proptypes, the properties className, onBlur, onFocus, and onKeyDown are removed from TreeView.propTypes.

When replacing (for testing):

const TreeView = React.forwardRef(function TreeView(inProps, ref) {
const { isRtl, ...props } = useThemeProps({ props: inProps, name: 'MuiTreeView' });
by:

const TreeView = React.forwardRef(function TreeView(props, ref) {
yarn proptypes adds the 4 properties to TreeView.propTypes.

I try to understand why this is happening, but till now without success. Do you have any suggestion for me where to look at?

Taking a look.

@mnajdova
Copy link
Member

mnajdova commented Apr 9, 2021

@tomasznguyen fixed the proptypes by c295e40

tomasznguyen and others added 2 commits April 9, 2021 22:07
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
tomasznguyen and others added 2 commits April 10, 2021 08:52
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
@tomasznguyen
Copy link
Contributor Author

@mnajdova @oliviertassinari Thank you very much for your feedback and reviews. All tests are passing now.

Based on your reviews, I reviewed my other migration PRs. I see that index.d.ts of those PRs need to be updated as well. I'll fix that. Furthermore, in Menu, I used destructuring to extract isRtl as in TreeView which resulted in className missing from proptypes. I'll fix this as well.

What I understand from migrating TreeView is that it is not recommended to destructure the output of useThemeProps at the moment, because this results in incorrectly generated proptypes, right?

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go :)

@mnajdova
Copy link
Member

Based on your reviews, I reviewed my other migration PRs. I see that index.d.ts of those PRs need to be updated as well. I'll fix that. Furthermore, in Menu, I used destructuring to extract isRtl as in TreeView which resulted in className missing from proptypes. I'll fix this as well.

Great! We may have missed that.

What I understand from migrating TreeView is that it is not recommended to destructure the output of useThemeProps at the moment, because this results in incorrectly generated proptypes, right?

Correct

@mnajdova mnajdova merged commit 13d5d4f into mui:next Apr 10, 2021
@tomasznguyen tomasznguyen mentioned this pull request Apr 10, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants